-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Report error when finding variadic function other than printf #2703
Report error when finding variadic function other than printf #2703
Conversation
Signed-off-by: Marcos Maronas <[email protected]>
Signed-off-by: Marcos Maronas <[email protected]>
lib/SPIRV/SPIRVWriter.cpp
Outdated
@@ -814,6 +814,11 @@ SPIRVType *LLVMToSPIRVBase::transSPIRVOpaqueType(StringRef STName, | |||
SPIRVType *LLVMToSPIRVBase::transScavengedType(Value *V) { | |||
if (auto *F = dyn_cast<Function>(V)) { | |||
FunctionType *FnTy = Scavenger->getFunctionType(F); | |||
// VarArg functions other than printf are not supported in SPIR-V. | |||
BM->getErrorLog().checkError( | |||
!(FnTy->isVarArg() && F->getName() != "printf"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test too strict? In the test array_of_matrices.ll, I see:
declare dso_local spir_func i32 @_Z18__spirv_ocl_printfPU3AS2Kcz(ptr addrspace(2), ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, there is mangling, and at least 3 names for the printf, so F->getName() != "printf"
would not work properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered F->getName().contains("printf")
, but thought that was too loose. What do you think, would that be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In SPIR-V Backend we have something along the lines of
if ((DemangledName.starts_with("__spirv_ocl_printf(") ||
DemangledName.starts_with("printf(")) ...
so I guess as a first approach this could work after you've got DemangledName
from F->getName()
from llvm::itaniumDemangle
.
Signed-off-by: Marcos Maronas <[email protected]>
Signed-off-by: Marcos Maronas <[email protected]>
; RUN: not llvm-spirv %t.bc 2>&1 | FileCheck %s | ||
|
||
target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-n8:16:32:64-G1" | ||
target triple = "spir64_unknown_unknown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spir64-unknown-unknown
|
||
define i32 @foo() nounwind { | ||
call spir_func void (i32, i32, ...) @for__issue_diagnostic(i32 noundef 41, i32 noundef 0) | ||
ret i32 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two spaces before ret :)
DemangledName.starts_with("__spirv_ocl_printf(")), | ||
SPIRVEC_UnsupportedVarArgFunction); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Marcos Maronas <[email protected]>
lib/SPIRV/SPIRVWriter.cpp
Outdated
(DemangledName.starts_with("printf(") || | ||
DemangledName.starts_with("__spirv_ocl_printf(")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oclIsBuiltin demangles "printf" to "__spirv_ocl_printf". I think only checking for: DemangledName.starts_with("__spirv_ocl_printf") should be sufficient. I think we should possibly be able to check for equality instead of starts_with.
Also does DemangledName include an open parenthesis?
oclIsBuiltin starts with:
SPIRVUtil.cpp:bool oclIsBuiltin(StringRef Name, StringRef &DemangledName, bool IsCpp) {
SPIRVUtil.cpp- if (Name == "printf") {
SPIRVUtil.cpp- DemangledName = "__spirv_ocl_printf";
SPIRVUtil.cpp- return true;
SPIRVUtil.cpp- }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have actually realized none of printf
variants can reach this piece of code because they are skipped in
SPIRV-LLVM-Translator/lib/SPIRV/SPIRVWriter.cpp
Line 5789 in 7dacb7c
if (isBuiltinTransToInst(&F) || isBuiltinTransToExtInst(&F) || |
isBuiltinTransToExtInst(&F)
returns true for printf
, so it is just skipped and never added to neither Decls
nor Defs
. Considering this, I think we should be safe by just checking if the function is variadic to report the error at this point. What do you think? Also tagging the rest of approvers to gather their opinions: @svenvh @MrSidims @VyacheslavLevytskyy
Signed-off-by: Marcos Maronas <[email protected]>
Signed-off-by: Marcos Maronas <[email protected]>
Signed-off-by: Marcos Maronas <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Marcos Maronas <[email protected]>
Variadic functions are not supported in SPIR-V, the only known exception is printf.